-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass-through postgres connection option 'statement_timeout' #8342
Conversation
node-postgres recently added support for 'statement_timeout' -- a per connection query timeout. See - brianc/node-postgres#1436
@amasad, thanks for your PR! By analyzing the history of the files in this pull request, we identified @felixfbecker, @janmeier and @mickhansen to be potential reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a pending PR which handles this already #8254. PR tries to test statement_timeout
but fails because Sequelize doesn't fully support pg@v7
.
I think we should add this flag without testing statement_timeout
, let pg worry about it. This will allow those on pg@v7
to use this feature.
cc @sequelize/core / @sequelize/trusted-contributors
Yes, I just realized that v7 isn't supported after trying to run it and hit the #7998 bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sushantdhiman Yep, agreed
That being said, wouldnt we want to hold off on this PR? Also, not a huge fan of adding dialect specific options to abstract components. |
Dialect specific option so there should be no problem. Sequelize is incompatible with MULTIPLE statements type, this option not related
Not sure I understood what you meant by abstract component, this is Postgres specific connection manager. This is a good place to set such options, we already have many pg specific options here like We are not actually adding it, we are white listing it so users can pass it as |
Model.upsert() fails with |
@jkennedy1980 No we dont have a timeframe. Its an open source project, when someone is free enough to work on it, fix will land. Till then just wait :) |
And you guys are sure you don't want to accept PR8029: #8029 - It basically converts the multiple results to the same thing you get with |
That PR attempts to solve it but its not a good solution. I haven't had chance to review that PR but as referred in #7998 we need a way to have MULTIPLE type statement define which result they need. If someone is willing to solve that issue I can explain in details what we need |
Ok. It seems that you have ideas about how you want the API to change to support multiple results. There are many issues/PRs open where people are proposing different solutions. I think it's time that you document what you're thinking about the API so that the community can discuss it and maybe even implement it. This would prevent people wasting their time on undesired solutions. I may even choose to implement if there is a good API design in place. |
node-postgres recently added support for 'statement_timeout' -- a per connection query timeout.
See brianc/node-postgres#1436